Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

scripts/build-motion-library-v5.py (3)

183-183: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Use with statements for file reads to prevent resource leaks.

json.load(open(...)) leaks the file handle. In CPython, reference counting closes it immediately, but this is not guaranteed on other implementations and can exhaust handles on Windows with large corpora.

♻️ Proposed fix

     if os.path.exists(mpath):
-        manifest = json.load(open(mpath))
+        with open(mpath) as f:
+            manifest = json.load(f)

-                        dump = json.load(open(tmp))
+                        with open(tmp) as f:
+                            dump = json.load(f)

Also applies to: 214-214

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/build-motion-library-v5.py` at line 183, Replace the direct open
calls used by the manifest loads in the relevant code paths with with open(...)
as ... context managers, then pass the managed file object to json.load; update
both occurrences near the manifest-loading logic.

---

102-104: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Add strict=True to zip() for a cheap safety guard.

If two quaternion arrays ever have mismatched lengths, zip silently truncates. Adding strict=True (Python 3.10+) turns that into a clear error.

♻️ Proposed fix

     d = abs(sum(x * y for x, y in zip(a, b, strict=True)))

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/build-motion-library-v5.py` around lines 102 - 104, Update quat_angle
to call zip(a, b, strict=True) when computing the quaternion dot product,
ensuring mismatched array lengths raise an explicit error instead of being
silently truncated.

---

215-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Log a warning when a corpus file fails instead of silently continuing.

The bare except Exception: continue hides qtmesh crashes, JSON decode errors, and IO failures. For an offline tool processing hundreds of files, this can silently skip valid assets and make debugging very difficult.

♻️ Proposed fix

                     except Exception as exc:
+                        print(f"  ! skipped {fn}: {exc}", file=sys.stderr)
                         continue

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/build-motion-library-v5.py` around lines 215 - 216, In the
corpus-file processing exception handler, replace the silent `except Exception:
continue` with a warning log that includes the affected file path and exception
details, then continue processing subsequent files. Use the surrounding
corpus-processing function and its existing logger or warning mechanism to
report failures consistently.

src/AnimationMerger.cpp (1)

1020-1024: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Precompute C.Inverse() outside the per-joint loop.

C is constant once derived, but C.Inverse() is recomputed for every joint in every frame. Precompute it once before the sampling loop for clarity.

♻️ Proposed refactor

     // Sample by applying each Animation DIRECTLY to the skeleton instance —
     // deterministic regardless of the entity's animation-state bookkeeping
     // (state-set application proved instance-dependent for hand-built
     // skeletons), and it leaves the entity's enabled states untouched.
+    const Ogre::Quaternion Cinv = C.Inverse();
     for (unsigned short a = 0; a < skel->getNumAnimations(); ++a) {
@@ -1018,7 +1019,7 @@
                 Ogre::Bone* b = roleBone[static_cast<size_t>(j)];
                 if (!b) continue;
                 const Ogre::Quaternion w =
-                    C * b->_getDerivedOrientation() * C.Inverse();
+                    C * b->_getDerivedOrientation() * Cinv;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/AnimationMerger.cpp` around lines 1020 - 1024, Precompute the constant
inverse of C once before the per-joint sampling loop, then reuse that value in
the orientation calculation within the loop in AnimationMerger’s pose-generation
logic, replacing each repeated C.Inverse() call.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

scripts/build-motion-library-v5.py (3)

203-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Guard a missing raw/ directory.

If the corpus is present but raw/ is absent, os.listdir(raw) raises an uncaught FileNotFoundError traceback instead of the intended friendly no clips extracted — is the corpus downloaded/validated? guidance. A quick os.path.isdir(raw) check before the walk keeps the failure message actionable.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/build-motion-library-v5.py` around lines 203 - 204, Update the
corpus-processing flow around the raw directory iteration to check
os.path.isdir(raw) before calling os.listdir(raw). When raw is absent, return or
trigger the existing friendly “no clips extracted — is the corpus
downloaded/validated?” guidance instead of allowing FileNotFoundError to escape;
preserve the current sorted walk for valid directories.

---

225-243: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Silent skips hide extraction failures — emit a diagnostic before continue.

Both the cache-parse fallback (Lines 227-230) and the qtmesh invocation (Lines 231-243) swallow every failure and continue with no output. Across a large corpus, a systematically failing qtmesh (bad binary, crash, non-zero exit, timeout) produces zero clues until the final no clips extracted message, forcing a blind rerun. A one-line warning to stderr (including r.stderr on non-zero exit) makes the failure mode diagnosable without changing control flow.

Note: Ruff's S603/subprocess-from-request and the open(cache) path-traversal hints are false positives here — qtmesh is a resolved local binary and cache/fpath derive from a locally-walked corpus tree, not external request input.

🔎 Suggested diagnostics

                     if dump is None:
                         try:
                             r = subprocess.run(
                                 [qtmesh, "anim", fpath,
                                  "--dump-canonical", cache],
                                 capture_output=True, text=True, timeout=600)
                             if r.returncode != 0 \
                                     or not os.path.exists(cache) \
                                     or not os.path.getsize(cache):
+                                print(f"  ! skip {fpath}: qtmesh rc={r.returncode} "
+                                      f"{r.stderr.strip()[:200]}", file=sys.stderr)
                                 continue
                             dump = json.load(open(cache))
-                        except Exception:
+                        except Exception as ex:
+                            print(f"  ! skip {fpath}: {ex}", file=sys.stderr)
                             continue

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/build-motion-library-v5.py` around lines 225 - 243, Add concise
stderr warnings before both silent continue paths in the cache JSON fallback and
qtmesh extraction handling, identifying the relevant cache/input file and
exception or failure context. For non-zero qtmesh exits, include r.stderr in the
diagnostic; preserve the existing control flow and skip behavior.

Source: Linters/SAST tools

---

196-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer a context manager (and explicit encoding) for the JSON reads.

json.load(open(mpath)) (also Lines 228 and 241) relies on CPython refcounting to close the descriptor and inherits the platform-default locale encoding. Using with open(mpath, encoding="utf-8") as f: closes deterministically and keeps parsing reproducible across platforms, matching the with open(args.out, "w") pattern already used at Line 296.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/build-motion-library-v5.py` around lines 196 - 198, Update the JSON
reads in the manifest-loading logic, including the reads around mpath and the
corresponding locations at lines 228 and 241, to use a with-open context manager
with explicit UTF-8 encoding before passing the file handle to json.load.
Preserve the existing parsing behavior while ensuring every opened input file is
deterministically closed.

src/AnimationMerger.cpp (1)

960-997: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated source/target-frame derivation math.

deriveFrame (inside extractCanonicalClips) and the Ct computation inside readTargetBindFrame implement the same hip→head/left-right→canonical-basis geometry almost verbatim. Extracting a shared helper (e.g., deriveCanonicalConjugation(hipPos, headPos, lPos, rPos)) would remove the duplication and prevent the two derivations from silently drifting if the geometry heuristic is ever tuned.

Also applies to: 1219-1250

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/AnimationMerger.cpp` around lines 960 - 997, The canonical frame geometry
is duplicated between the deriveFrame lambda in extractCanonicalClips and the Ct
calculation in readTargetBindFrame. Extract the shared hip/head/left-right basis
construction and quaternion conversion into a reusable helper such as
deriveCanonicalConjugation, then have both call sites supply their bone
positions and reuse its fallback/degenerate handling and identity result.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes